Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: destructuring of structs and messages #856

Merged
merged 30 commits into from
Oct 15, 2024

Conversation

Gusarich
Copy link
Member

Issue

Closes #298.

Checklist

  • I have updated CHANGELOG.md
  • I have documented my contribution in Tact Docs: https://github.com/tact-lang/tact-docs/pull/PR-NUMBER
  • I have added tests to demonstrate the contribution is correctly implemented: this usually includes both positive and negative tests, showing the happy path(s) and featuring intentionally broken cases
  • I have run all the tests locally and no test failure was reported
  • I have run the linter, formatter and spellchecker
  • I did not do unrelated and/or undiscussed refactorings

@Gusarich Gusarich added this to the v1.6.0 milestone Sep 20, 2024
@Gusarich Gusarich requested a review from a team as a code owner September 20, 2024 09:04
src/interpreter.ts Outdated Show resolved Hide resolved
@anton-trunov anton-trunov self-assigned this Sep 23, 2024
Copy link
Member

@anton-trunov anton-trunov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comment for the grammar.ohm file

src/grammar/grammar.ohm Outdated Show resolved Hide resolved
@Gusarich
Copy link
Member Author

Gusarich commented Oct 8, 2024

@jeshecdom @anton-trunov have a look at the reworked apporach please!

@anton-trunov
Copy link
Member

@Gusarich There are some merge conflicts

@Gusarich
Copy link
Member Author

Gusarich commented Oct 8, 2024

@Gusarich There are some merge conflicts

Resolved them, trying to push but knip doesn't let me because for some reason it thinks that @types/jest is not used. What should I do?

@anton-trunov
Copy link
Member

Resolved them, trying to push but knip doesn't let me because for some reason it thinks that @types/jest is not used. What should I do?

hmmm, this is weird, because I removed that from ignoreDependencies (see the getter method id pr) precisely because I was getting warnings that it is useless

can you try updating your knip installation locally?

@Gusarich
Copy link
Member Author

Gusarich commented Oct 8, 2024

can you try updating your knip installation locally?

yeah, just had to run yarn. didn't notice the change in package.json. have a look now

Copy link
Member

@anton-trunov anton-trunov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a few comments and suggestions for the changes. Since this is a complex feature (yeah, codegen is quite simple, but the static semantics is a bit tricky), we need to test it more thoroughly, so let's add at least the following tests:

  • positive interpreter (const-eval) tests (including various combinations of punned and mapped fields);
  • negative interpreter tests;
  • negative typechecker tests with the scoping rules violations, like let S {x: x, y: x} and shadowing of previously defined variables;
  • negative typechecker tests with all the new error messages (that are non-ICE ones);
  • pretty-printer tests;
  • renamer tests;

src/grammar/grammar.ohm Outdated Show resolved Hide resolved
src/grammar/ast.ts Outdated Show resolved Hide resolved
src/grammar/grammar.ohm Outdated Show resolved Hide resolved
src/grammar/ast.ts Outdated Show resolved Hide resolved
src/grammar/test/stmt-destructuring.tact Outdated Show resolved Hide resolved
src/test/e2e-emulated/contracts/structs.tact Show resolved Hide resolved
@anton-trunov
Copy link
Member

anton-trunov commented Oct 9, 2024

@jeshecdom Please have a look at the interpreter changes and suggest more positive and negative tests. And also, please help with the var-scoping and typechecking rules review

@anton-trunov
Copy link
Member

  1. Also, let's document this feature: look at how it's done in PR feat: specify method ID for getters #922 ("available since").

  2. And let's add some more negative tests for the typechecker in this style: let S1 {x, y} = S2 {x: 42, y: 43}

@novusnota
Copy link
Member

novusnota commented Oct 10, 2024

Also, let's document this feature: look at how it's done in PR #922 ("available since").

If needed, I can quickly help write the docs once all the other suggestions above are resolved :)

@Gusarich
Copy link
Member Author

Gusarich commented Oct 11, 2024

@anton-trunov @jeshecdom I think I've made all the changes you requested in the review. Please have a look!

Discovered and fixed a few small bugs while writing all these tests 👍

Copy link
Contributor

@jeshecdom jeshecdom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

src/generator/writers/writeFunction.ts Outdated Show resolved Hide resolved
src/test/e2e-emulated/contracts/structs.tact Outdated Show resolved Hide resolved
src/types/__snapshots__/resolveStatements.spec.ts.snap Outdated Show resolved Hide resolved
src/types/__snapshots__/resolveStatements.spec.ts.snap Outdated Show resolved Hide resolved
src/grammar/grammar.ts Outdated Show resolved Hide resolved
src/grammar/ast.ts Outdated Show resolved Hide resolved
@Gusarich
Copy link
Member Author

Gusarich commented Oct 14, 2024

@anton-trunov resolved everything, have a look please!

src/grammar/ast.ts Outdated Show resolved Hide resolved
Co-authored-by: Novus Nota <[email protected]>
@anton-trunov anton-trunov merged commit ce201fb into tact-lang:main Oct 15, 2024
15 checks passed
@anton-trunov
Copy link
Member

anton-trunov commented Oct 15, 2024

If needed, I can quickly help write the docs once all the other suggestions above are resolved :)

@novusnota your turn :)

@novusnota
Copy link
Member

novusnota commented Oct 15, 2024

@anton-trunov on it!

UPD: #964

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Simple destructuring of fields of structs and messages
4 participants